Skip to content

Migrate jetpack_published_post to wp_after_insert_post hook #17838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mdbitz merged 2 commits into from
Nov 23, 2020
Merged

Migrate jetpack_published_post to wp_after_insert_post hook #17838

mdbitz merged 2 commits into from
Nov 23, 2020

Conversation

@mdbitz
Copy link
Contributor

@mdbitz mdbitz commented Nov 18, 2020 •
edited by jeherve
Loading

WP 5.6 introduces the new wp_after_insert_post function and action that is triggered after a post and its terms, meta have been saved. We are migrating the jetpack_published_post sync event to this new hook to ensure that Publicize, Subscriptions and Elastic based functionality has the complete data available to it when they are triggered.

Fixes #14629
Fixes #11752
See: https://core.trac.wordpress.org/changeset/49172
See: https://make.wordpress.org/core/2020/11/20/new-action-wp_after_insert_post-in-wordpress-5-6/

Changes proposed in this Pull Request:

  • Migrate jetpack_published_post sync event to trigger on wp_after_insert_post instead of wp_insert_post.

Jetpack product discussion

Historically WP.com dependent Jetpack functionality has run into issues with race conditions or chunked sync event processing that has lead to Publicize, Subscription Emails, Related Posts, Search being triggered that does not contain the full terms and meta data. This missing data would in several cases have caused different flows in processing.

This migration to wp_after_insert_post allows us to ensure that publish actions have the full context of the post and its term/meta data so we are confidently triggering the proper action across all functionality.

Does this pull request change what data or activity we track or use?

No, the same content is being synced.

Testing instructions:

This change requires a full regression of functionality tied to jetpack_published_post

  • Publicize. -- Are social posts being created
  • Subscription Emails -- see Instant Search: Add Jetpack Search gridicon #14629 for test instructions
  • Related Posts - post is in global index
  • Likes - post in global index (otherwise likes don't render)
  • Instant Search - jetpack-search index has correct terms,meta when publishing a post
  • Activity Log - 3 activities reference this action
  • Backups - latest post info is captured

Specific Instructions
Confirm jetpack_published_post is being sent in 5.6

  • Update your site to use latest 5.6 release candidate
  • Add a post to your site
  • Verify jetpack_published_post action is queued and sent via Sync

Confirm jetpack_published_post is being sent still for versions less than 5.6

  • Update your site to use WP 5.5.*
  • Add a post to your site
  • Verify jetpack_published_post action is queued and sent via Sync

Note there are several methods to testing sync events. They are outlined at : PCYsg-svc-p2

Proposed changelog entry for your changes:

  • Jetpack Sync :: Migration of jetpack_published_post action to new 5.6 hook wp_after_insert_post

@mdbitz
Migrate jetpack_published_post to wp_after_insert_post hook if wp_aft…
95a8ab7 
…er_insert_post function is defined. Added to-do to cleanup wp_insert_post flow once 5.6 is the minimal supported version.
@mdbitz mdbitz added [Feature] Publicize Now Jetpack Social, auto-sharing [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Status] In Progress [Package] Sync [Feature] Search For all things related to Search [Feature] Activity Log [Feature] Backups labels Nov 18, 2020
@mdbitz mdbitz added this to the 9.2 milestone Nov 18, 2020
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Nov 18, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Nov 18, 2020 •
edited
Loading

Scheduled Jetpack release: December 1, 2020.
Scheduled code freeze: November 23, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17838

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 140bd35

@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 18, 2020

Notes on Testing.

The wp_after_insert_post function is covered by existing tests:
test_sync_jetpack_update_post_to_draft_shouldnt_publish
test_sync_jetpack_published_post_raw
test_sync_jetpack_publish_post_works_with_interjecting_plugins

However it doesn't include cases where it should early return. We should have unit tests that manually calls wp_after_insert_hook with the following cases
$post_ID null, string. -> no jetpack_published_post event
$post null -> no jetpack_published_post event
$post_ID non existing ID -> no jetpack_published_post event

@mdbitz mdbitz added the [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. label Nov 18, 2020
@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 18, 2020

Current E2E (All & with latest Gutenberg) Test Failure is a connection timeout unrelated to these changes.

@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 18, 2020

Notes on Testing.

The wp_after_insert_post function is covered by existing tests:
test_sync_jetpack_update_post_to_draft_shouldnt_publish
test_sync_jetpack_published_post_raw
test_sync_jetpack_publish_post_works_with_interjecting_plugins

However it doesn't include cases where it should early return. We should have unit tests that manually calls wp_after_insert_hook with the following cases
$post_ID null, string. -> no jetpack_published_post event
$post null -> no jetpack_published_post event
$post_ID non existing ID -> no jetpack_published_post event

Testing is being done in test_sync_jetpack_published_post_no_action with a data provider that covers the above 4 cases.

@mdbitz mdbitz added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Tests] Needs Tests Some Unit Tests would be really useful to include with this PR. labels Nov 18, 2020
anomiex
anomiex reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code itself looks sane, and I see it has some tests which Travis is passing.

The instructions don't make it clear to me what I'm supposed to do manually, how do I "Verify jetpack_published_post action is queued and sent via Sync"? Should some instructions be being added to to-test.md for beta testers?

@anomiex anomiex added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Nov 19, 2020
@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 20, 2020

@kraftbj @jeherve, @anomiex what level of detail for testing do you think will be appropriate, does this deserve its own call to action for regression? Or should it be in the to-test instructions to verify posts are publicized to social media and that new posts show up in Search Results.

Verifying 1 action triggers validates that the jetpack_published_post event is still being triggered.

@jeherve
Copy link
Member

jeherve commented Nov 23, 2020

should it be in the to-test instructions to verify posts are publicized to social media and that new posts show up in Search Results.

I think such detailed instructions for Beta testers would be nice, and we'll get folks to test with both 5.5 and 5.6.

mdbitz added a commit that referenced this pull request Nov 23, 2020
@mdbitz
Testing instructions for Sync wp_after_insert_post migration #17838
8704085
@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 23, 2020

Sync Testing instructions have been outlined in https://github.com/Automattic/jetpack/pull/17772/files

@mdbitz mdbitz added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Nov 23, 2020
anomiex
anomiex approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemed to work using Jetpack's "Instant Search" as the testing method.

Note the code freeze is today, so if you want this in 9.2 you should get it merged ASAP.

@anomiex anomiex removed the [Status] Needs Review This PR is ready for review. label Nov 23, 2020
@anomiex anomiex added the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 23, 2020
@mdbitz mdbitz merged commit fb22af4 into master Nov 23, 2020
@mdbitz mdbitz deleted the sync/wp_after_insert_post branch November 23, 2020 16:02
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 23, 2020
@mdbitz
Copy link
Contributor Author

mdbitz commented Nov 23, 2020

Thanks for the review and feedback everyone.

This was referenced Nov 23, 2020
Closed
Open
anomiex added a commit that referenced this pull request Nov 23, 2020
@anomiex
Changelog: add #17838
2fa6ec9
@kraftbj kraftbj mentioned this pull request Jun 11, 2021
Closed
@jeherve jeherve mentioned this pull request Dec 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anomiex anomiex anomiex approved these changes

Assignees

No one assigned

Labels

[Feature] Activity Log [Feature] Backups [Feature] Publicize Now Jetpack Social, auto-sharing [Feature] Search For all things related to Search [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Package] Sync [Status] Needs Package Release This PR made changes to a package. Let's update that package now.

Projects

None yet

Milestone

9.2

Development

Successfully merging this pull request may close these issues.

Publicize Custom Message Doesn't Show In Twitter

6 participants

@mdbitz @jetpackbot @jeherve @anomiex @matticbot